Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load timer #405

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Load timer #405

wants to merge 22 commits into from

Conversation

Tearran
Copy link
Member

@Tearran Tearran commented Feb 1, 2025

Description

refinement of #326

Added a Timer function to help identify sections that are slow and may need optimization or separation to improve performance.

./tools/armbian-config-dev and ./bin/armbian-config

Initializing script: 0 seconds
Setting base data: 0 seconds
The current Armbian 24.8.4 stable (bookworm) is supported.: 4 seconds
Loaded Runtime variables...: 0 seconds
Loaded Dialog...: 0 seconds
Loaded Docs...: 0 seconds
Loaded System helpers...: 0 seconds
Loaded Network helpers...: 0 seconds
Loaded Software helpers...: 0 seconds

note '--api' '--docs' and '! -t =$0" do not output timer results see

./bin/armbian-config --api set_checkpoint help

Usage: set_checkpoint <start|stop|mark|show> [description] [show]
Commands:
  start              Start the timer.
  stop               Stop the timer.
  mark [description] [time] Mark a checkpoint with an optional description and an optional flag to show the output.
  show               Show the total elapsed time and checkpoints.

Issue reference:
[Bug]: sanitize_input #324
function raceing
Partly addresses UX vs Non interactive mode use

Implementation Details

  • added variable checks for UX mode to help separate from the non interactive modes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have ensured that my changes do not introduce new warnings or errors
  • No new external dependencies are included
  • Changes have been tested and verified
  • I have included necessary metadata in the code, including associative arrays

@github-actions github-actions bot added the size/large PR with 250 lines or more label Feb 1, 2025
@Tearran Tearran marked this pull request as ready for review February 1, 2025 20:48
@dimitry-ishenko
Copy link
Collaborator

dimitry-ishenko commented Feb 2, 2025

@Tearran IMHO it would be PITA to maintain two copies of armbian-config.

What about using an env var (eg, ARMBIAN_CONFIG_DEBUG or just DEBUG) to show the timing info? Eg,

DEBUG=1 ./bin/armbian-config ...

Then in message_checkpoint.sh you can add something like this at the end:

# disable set_checkpoint if DEBUG has not been defined
[[ -n "$DEBUG" ]] || eval "set_checkpoint(){ :; }"

@igorpecovnik
Copy link
Member

IMHO it would be PITA to maintain two copies

Yes, I would also propose to have one.

@igorpecovnik igorpecovnik added 02 Milestone: First quarter release Needs review Seeking for review labels Feb 3, 2025
remove redundant file
@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines and removed size/large PR with 250 lines or more labels Feb 5, 2025
@github-actions github-actions bot added size/large PR with 250 lines or more and removed size/medium PR with more then 50 and less then 250 lines labels Feb 5, 2025
@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines and removed size/large PR with 250 lines or more labels Feb 5, 2025
dimitry-ishenko
dimitry-ishenko previously approved these changes Feb 5, 2025
Copy link
Collaborator

@dimitry-ishenko dimitry-ishenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tearran I was under the impression that we only wanted timing info for debugging purposes. I kinda like that it's printed every time, but I don't know if there might be objections in terms of performance or some other reasons.

bin/armbian-config Outdated Show resolved Hide resolved
tools/modules/initialize/message_checkpoint.sh Outdated Show resolved Hide resolved
tools/modules/initialize/message_checkpoint.sh Outdated Show resolved Hide resolved
@Tearran
Copy link
Member Author

Tearran commented Feb 5, 2025

What about using an env var (eg, ARMBIAN_CONFIG_DEBUG or just DEBUG) to show the timing info?

@dimitry-ishenko I am not opposed to using a DEBUG environment variable in general. However, I would like to clarify that the messages displayed are not meant for debugging purposes and are not being introduced with this PR. These messages were previously implemented to improve the user experience by indicating that the application is working and not frozen.

In other words, this PR does not address bugs. Rather, it outlines the expected parsing load time limitations due to the linear nature of bash, such as function variable racing and other inherent constraints.

The addition of the timestamp is multifaceted. Firstly, IMHO it is important to recognize the limitations of bash, so that expected limitations are not identified as bugs. Showing loading time addresses this by clearly showing expected behavior. For example, 4 seconds may be undesirable but it is not a bug. By providing these user-facing messages, we aim to enhance user experience by transparently communicating the application's activity, thus avoiding any misconceptions of the application being unresponsive.

@dimitry-ishenko
Copy link
Collaborator

@dimitry-ishenko I am not opposed to using a DEBUG environment variable in general. However, I would like to clarify that the messages displayed are not meant for debugging purposes and are not being introduced with this PR. These messages were previously implemented to improve the user experience by indicating that the application is working and not frozen.

@Tearran sorry, not sure why I got the idea this was for debugging. Thanks for the clear explanation, it makes sense.

bin/armbian-config Outdated Show resolved Hide resolved
@Tearran
Copy link
Member Author

Tearran commented Feb 7, 2025

Sorry, not sure why I got the idea this was for debugging.

no need to apologize, Debugging is a reasonable conclusion; the armbian-config-dev option does resemble debugging. and set_checkpoint option show is intended to serve a debugging purpose , That being said the Discussion of Debugging is valid while it is mindfully relative and IMO is applicable here

bin/armbian-config Outdated Show resolved Hide resolved
@Tearran
Copy link
Member Author

Tearran commented Feb 8, 2025

Added alias/helper checkpoint with a debug option.
Use in file: checkpoint debug "Starting --api options"
For more, see checkpoint help (set_checkpoint help debug appended).
sudo ./bin/armbian-config --api checkpoint help

Usage: set_checkpoint <start|stop|mark|show> [description] [show]
Commands:
  start              Start the timer.
  stop               Stop the timer.
  mark [description] [time] Mark a checkpoint with an optional description and an optional flag to show the output.
  show               Show the total elapsed time and checkpoints.
  debug              DEBUG checkpoint message

Debug level 2 shows like UX mode, printing each step.

Other DEBUG levels (DEBUG=1, DEBUG=anything) default to set_checkpoint show.

This section doe not print with other DEBUG values
sudo DEBUG=2 ./bin/armbian-config --api checkpoint help

Initializing script                     0 seconds
Loaded Runtime variables...             4 seconds
The current Armbian 24.8.4 stable (bookworm) is supported.           0 seconds
Loaded Dialog...                        0 seconds
Loaded Docs...                          0 seconds
Loaded System helpers...                1 second
Loaded Network helpers...               0 seconds
Loaded Software helpers...              0 seconds
Loaded Runtime conditions...            5 seconds
DEBUG Messages on                       0 seconds
Starting --api options                  0 seconds

The normal output if any from the command line options.

Following shows if debug has a value [[ -n $DEBUG ]]

Exiting --api                           0 seconds
Total elapsed time            : 10 seconds
Initializing script           : 0 seconds
Loaded Runtime variables...   : 4 seconds
The current Armbian 24.8.4 stable (bookworm) is supported.: 0 seconds
Loaded Dialog...              : 0 seconds
Loaded Docs...                : 0 seconds
Loaded System helpers...      : 1 second
Loaded Network helpers...     : 0 seconds
Loaded Software helpers...    : 0 seconds
Loaded Runtime conditions...  : 5 seconds
DEBUG Messages on             : 0 seconds
Starting --api options        : 0 seconds
Exiting --api                 : 0 seconds
{C1E728F6-2397-4123-AF21-F3DD713B9AA2}

@dimitry-ishenko
Copy link
Collaborator

@Tearran the problem is not with the testcase, but something else:

Run export TERM=linux
All scripts have been combined and placed into tools/../lib/armbian-config
Processing JSON files, please wait...
JSON files rejoined into 'tools/../lib/armbian-config/config.jobs.json'.
Markdown files created in 'docs' directory, organized by top-level folders for both technical and user documentation.
 ____  _   _ _   _   _____ _____ ____ _____    ____    _    ____  _____ 
|  _ \| | | | \ | | |_   _| ____/ ___|_   _|  / ___|  / \  / ___|| ____|
| |_) | | | |  \| |   | | |  _| \___ \ | |   | |     / _ \ \___ \|  _|  
|  _ <| |_| | |\  |   | | | |___ ___) || |   | |___ / ___ \ ___) | |___ 
|_| \_\\___/|_| \_|   |_| |_____|____/ |_|    \____/_/   \_\____/|_____|
                                                                        
docker: Error response from daemon: Conflict. The container name "/portainer" is already in use by container "c1217285613f07ce14c18731c0856c38778d8964d7ea1141b9a0574bd9b25f3a". You have to remove (or rename) that container to be able to reuse that name.
See 'docker run --help'.
Error: Process completed with exit code 1.

@igorpecovnik
Copy link
Member

but something else:

yeah, ignore that. Will fix when possible

revert module_cockpit test case
@dimitry-ishenko
Copy link
Collaborator

@Tearran just wanted to say, I like where you are going with that 😃

@Tearran
Copy link
Member Author

Tearran commented Feb 9, 2025

Link to Youtube, showing DEBUG and loading messages
IMAGE ALT TEXT HERE

@dimitry-ishenko
Copy link
Collaborator

dimitry-ishenko commented Feb 9, 2025

Off-topic, but wow, it takes 5 seconds to load runtime vars...

@Tearran Tearran added the Ready to merge Reviewed, tested and ready for merge label Feb 9, 2025
@Tearran
Copy link
Member Author

Tearran commented Feb 9, 2025

This is about all i got for this pr, Unless there are other suggestion I am happy to receive, I believe this is ready to meagre. :)

Off-topic, but wow, it takes 5 seconds to load runtime vars...

IMO Related-topic. Also related is, ./tools/armbian-config-dev, ./tools/config-assemble, and

just wanted to say, I like where you are going with that 😃

Thanks for saying.

or more precisely, parsing in general, are all related to taking steps to sort and be clear this is expected albe it undesirable behavior, as well as some other minor compounding issues that are clearly becoming major. This thing got huge :)

I've been exploring this idea ConfigNG. Unifying the paring data to one source type so we can to output the others. in part to remove the need of the current runtme.sh jq updates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02 Milestone: First quarter release Needs review Seeking for review Ready to merge Reviewed, tested and ready for merge size/medium PR with more then 50 and less then 250 lines Unit Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants